feat(garm): drain resources before application removal - #312
Conversation
|
|
||
| cleanup_cls.assert_called_once_with(garm_api.auth_client) | ||
| cleanup_cls.return_value.run.assert_called_once_with() | ||
| assert out is not None |
There was a problem hiding this comment.
just a question, is there a way to assert the state, ie that the actual resource is removed, rather than just asserting that a method is called?
There was a problem hiding this comment.
Yup, that would be:
- write a fake client with internal state
- inject the fake client and assert the internal state after testing.
However, I think this isn't quite worth the effort here because we may end up testing the fake client's implementation.
| self._reconcile, | ||
| ) | ||
| self.framework.observe(self.on.update_status, self._reconcile) | ||
| self.framework.observe(self.on.remove, self._on_remove) |
There was a problem hiding this comment.
can we add this removal flow to an existing integration test (if any)
There was a problem hiding this comment.
Good idea, although I think we may have skipped that to reduce the cost of heavy integration test. If it doesn't exist, i'll test it via staging.
There was a problem hiding this comment.
Pull request overview
This PR wires the existing GARM resource-drain orchestration into the garm charm’s on.remove hook so that scalesets/runners are cleaned up before Juju removes the application, and removal is refused when admin credentials are unavailable.
Changes:
- Observe
on.removeand runGarmResourceCleanupusing an authenticated local GARM API client. - Fail the remove hook when admin credentials are missing to avoid orphaning external resources.
- Add Scenario unit tests covering the remove-hook cleanup invocation and the missing-credentials failure path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
charms/garm/src/charm.py |
Adds on.remove observer and executes authenticated cleanup prior to charm removal; refuses removal when admin credentials aren’t available. |
charms/garm/tests/unit/test_charm.py |
Adds unit tests asserting cleanup is invoked on remove and that removal fails without credentials. |
Suppressed comments (1)
charms/garm/tests/unit/test_charm.py:514
- This test’s docstring should follow the arrange/act/assert convention used throughout this file for readability and consistency.
def test_remove_refuses_without_admin_credentials(ctx: Context, garm_api: _GarmApiMocks):
"""Removal fails rather than orphaning resources when credentials are unavailable."""
with pytest.raises(UncaughtCharmError, match="credentials are unavailable"):
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def test_remove_runs_garm_cleanup_before_charm_termination(ctx: Context, garm_api: _GarmApiMocks): | ||
| """Application removal drains GARM resources before the charm is removed.""" |
| if not admin_creds: | ||
| raise RuntimeError("GARM admin credentials are unavailable; refusing removal") | ||
|
|
||
| base_url = f"http://127.0.0.1:{GARM_PORT}/api/v1" |
There was a problem hiding this comment.
Should the base_url be a constant?
a30f035 to
cb0aca4
Compare
Summary
on.removehook before application termination.Test plan
pytest -q tests/unit(238 passed)Stacked on #311 and #310 as the final ISD-6082 slice.
Refs: ISD-6082